Support multiple fee receiver accounts - #100
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change adds configurable fee receiver addresses. The server validates them against the on-chain ChangesFee receiver configuration and processing
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Server
participant FlowClient
participant FlowFees
participant Logger
Server->>FlowClient: Retrieve latest block
Server->>FlowClient: Execute get_fee_receivers
FlowClient->>FlowFees: Call getFeeReceiverAddresses()
FlowFees-->>FlowClient: Return receiver addresses
FlowClient-->>Server: Return Cadence result
Server->>Logger: Log validation result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@api/api.go`:
- Around line 108-109: Make successful fee-receiver validation a prerequisite
for readiness: update api/api.go lines 108-109 so Run either waits for
validateFeeReceivers before serving or keeps the service unready and rejects
construction requests until it succeeds. In api/validate.go lines 45-56 and 76,
propagate malformed validation results and retry exhaustion to that
startup/readiness gate instead of only logging them. Document the finalized
readiness behavior in README.md lines 377-380.
In `@api/validate.go`:
- Around line 24-26: Move the DataAccessNodes.Client() call from before the
retry loop into the loop around the existing attempt logic, so each retry
selects an access node independently while preserving the five-attempt behavior.
In `@go.mod`:
- Around line 14-16: Update the flow-go dependency pin in go.mod from commit
2799d6025ac3 to a current commit that contains upstream onflow/flow-go#8632,
specifically at or after merge commit b73fea20b2529a55c2bc3c3abb59a2ad891bba15;
alternatively, merge the prerequisite commit before retaining this change.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bc3db4ce-6190-4d11-bef1-0c0fc45a3f3c
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
README.mdapi/api.goapi/construction_service.goapi/validate.goconfig/config.goconfig/config_test.gogo.modscript/cadence/scripts/get-fee-receivers.cdcscript/script.goscript/script_test.gostate/process.gostate/state.gotestnet.json
| s.feeAddrs = s.Chain.Contracts.FeeAddresses() | ||
| go s.validateFeeReceivers(ctx) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make successful fee-receiver validation a readiness requirement.
Run starts validation in a goroutine and then starts the HTTP server. If configuration omits an active receiver, ConstructionPreprocess can accept a construction request for a transfer to that receiver before validation completes. A malformed result or exhausted retries also ends validation with only a log entry, so the service can continue without successful validation.
api/api.go#L108-L109: wait for successful validation before serving, or keep the service unready and reject construction requests until validation succeeds.api/validate.go#L45-L56: propagate malformed-result failures to the startup or readiness gate.api/validate.go#L76-L76: propagate retry exhaustion to the startup or readiness gate.README.md#L377-L380: document the finalized readiness behavior.
📍 Affects 3 files
api/api.go#L108-L109(this comment)api/validate.go#L45-L56api/validate.go#L76-L76README.md#L377-L380
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@api/api.go` around lines 108 - 109, Make successful fee-receiver validation a
prerequisite for readiness: update api/api.go lines 108-109 so Run either waits
for validateFeeReceivers before serving or keeps the service unready and rejects
construction requests until it succeeds. In api/validate.go lines 45-56 and 76,
propagate malformed validation results and retry exhaustion to that
startup/readiness gate instead of only logging them. Document the finalized
readiness behavior in README.md lines 377-380.
|
Notes on the CodeRabbit review threads: go.mod pin — false positive: Dependency updates — intentional: this PR needs flow-go with core-contracts v1.10.4 (the fee-receiver getter); cadence/sdk bumps ride along. Access node per retry — valid, applied in aac270d. Note the shipped configs run a single access node per spork, so in practice this is a consistency fix (every other call site already picks via Readiness gate — not taking this one. The dangerous case (an on-chain receiver missing from config) already ends in There is a genuine small gap nearby though: after 5 failed attempts, validation gives up for good with a single log line. A flaky access node at startup plus a stale config would leave the server running with an unvalidated fee set indefinitely. Follow-up idea for @janezpodhostnik (not a blocker): after the quick retries, fall back to a slow poll instead of returning, and/or surface the validation state via |
|
Tip For best results, initiate chat on the files or code changes.
I withdraw the The dependency updates are necessary for The I also withdraw the readiness-gate concern. Older FlowFees deployments can lack The slow-poll or 🐇 ✏️ Learnings added
You are interacting with an AI system. |
|
added a PR on top of this one to address some of the concerns: #101 |
Testnet distributes transaction fee deposits across multiple receiver accounts
since the FlowFees concurrent fee collection upgrade (rolled out in tx
be210889dd26a320f530595bd369093e866e26c3941bf7a3d01f861db3eeda81). Rosettaonly recognized deposits to the FlowFees contract account as fees, so deposits
to the child fee accounts were misclassified as ordinary transfers.
Changes
feeAddrwith a set of fee addresses(
Contracts.FeeAddresses()): the FlowFees account plus a new.contracts.fee_receiversconfig list, documented in the README.testnet.json, verified against theon-chain
FlowFees.getFeeReceiverAddresses()getter.validateFeeReceivers) that fetches the on-chainreceiver list via a new
get-fee-receiversscript and exits with a fatalerror if any on-chain receiver is missing from the config, so future
receiver additions fail loudly instead of silently misclassifying fees.
Extra configured addresses are allowed, since they may be needed to
classify fees in historical blocks.
getFeeReceiverAddresses()getter the script uses.Notes
mainnet.jsonis unchanged andbehavior there is identical. Once the upgrade rolls out there, the startup
check will flag the new receivers and
mainnet.jsonjust needs afee_receiversentry.yet merged. Do not merge this PR until it lands and the pin is updated to a
master commit.
Related: onflow/flow-core-contracts#575, onflow/flow-core-contracts#615, onflow/flow-go#8632
Summary by CodeRabbit
New Features
Documentation
fee_receiverssetting, address examples, fee behavior, and validation.Bug Fixes